Intrinsicify (Sse, Axv2, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan #41097

benaadams · 2020-08-20T15:48:42Z

Change the output wasm for IndexOfOrLessThan to be a simple for loop in C# chosen via packaging as suggested in #40705 (comment)

Update the vectorized path to sync with the SpanHelpers.Byte.cs method it is a variant of adding intrinsics for Sse, Axv2, Arm64 (upto +20% improvement for ReadJson on x64)

/cc @danroth27, @steveharter

benaadams · 2020-08-20T16:00:41Z

Correct artefacts are being generated

net5.0-Browser-Release

net5.0-Release

benaadams · 2020-08-20T16:01:16Z

x64 performance changes (up to +20% improvement)

better: 30, geomean: 1.113
worse: 1, geomean: 1.066
total diff: 31

| Slower                                                                           | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<SimpleStructWithProperties>.Deseri |      1.07 |           288.23 |           307.28 |         |

| Faster                                                                           | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.22 |         22031.18 |         18102.24 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.20 |         21554.46 |         17944.26 |         |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.20 |        341085.15 |        284277.87 |         |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromUtf |      1.19 |         29706.85 |         24903.98 |         |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.19 |        330481.64 |        277947.73 |         |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.19 |        399397.91 |        336085.63 |         |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr |      1.18 |         31986.63 |         27064.34 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.18 |         23919.94 |         20268.43 |         |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr |      1.18 |         31000.14 |         26348.41 |         |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr |      1.15 |           522.04 |           454.92 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromUtf8Byte |      1.12 |         68993.04 |         61636.12 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromUt |      1.10 |         12663.49 |         11477.42 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String,  |      1.10 |         78887.97 |         71927.75 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromStream   |      1.10 |         69968.66 |         63797.29 |         |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr |      1.09 |           763.93 |           698.34 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt |      1.09 |         12745.33 |         11734.38 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String |      1.09 |         48372.57 |         44572.25 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromStream   |      1.09 |         51673.60 |         47617.28 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String |      1.08 |         51226.78 |         47249.65 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt |      1.08 |         13979.59 |         12973.07 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String |      1.08 |         48102.61 |         44652.30 |         |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromStream  |      1.07 |           807.95 |           755.26 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromString   |      1.07 |         49947.46 |         46735.64 |         |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromUtf8Byt |      1.07 |           488.59 |           458.07 |         |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromUtf |      1.06 |           426.56 |           400.78 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String,  |      1.06 |         78155.94 |         73498.13 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromString   |      1.06 |         67906.38 |         63896.07 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromUtf8Byte |      1.06 |         50083.93 |         47157.52 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String,  |      1.05 |         73213.63 |         69703.93 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Int32>.DeserializeFromUtf8Bytes    |      1.03 |            90.42 |            87.57 |         |

eerhardt · 2020-08-20T16:41:47Z

@benaadams - Do we actually need to cross compile for Broswer to make this kind of change?

Blazor WASM apps are run through the ILLinker during publish. At link-time, the ILLinker is passed that SSE2.IsSupported, AdvSimd.Arm64.IsSupported, and Vector.IsHardwareAccelerated are all false statically so the ILLinker will link away any code that is conditioned by them.

So I think you should naturally write the code as you typically would:

            if (Sse2.IsSupported || AdvSimd.Arm64.IsSupported)
            {
                // intrinics code
            }
            else if (Vector.IsHardwareAccelerated)
            {
                // Vector code
            }
            else
            {
                 // normal software fallback
            }

And for a Blazor WASM app, all that will be in the published assembly will be the //normal software fallback code.

You don't need to build specifically for Browser to get "size opts". The tooling is set up to make it just work.

eng/testing/linker/trimmingTests.targets

eng/testing/linker/SupportFiles/Directory.Build.props

benaadams · 2020-08-20T16:56:12Z

Do we actually need to cross compile for Broswer to make this kind of change?

Blazor WASM apps are run through the ILLinker during publish. At link-time, the ILLinker is passed that SSE2.IsSupported, AdvSimd.Arm64.IsSupported, and Vector.IsHardwareAccelerated are all false statically so the ILLinker will link away any code that is conditioned by them.

I don't think the wasm likes the ref address taking or gotos in the main method; adding a branch to move to a simple for loop method then doesn't get eliminated for the non-wasm platforms so will regress them. Was a lot of discussion in
#39733 (comment) /cc @jkotas @steveharter, @ahsonkhan

benaadams · 2020-08-20T17:03:06Z

Adding an intrinsic to the mono interpreter for this method was also looked at #40705, but considered too fragile to do for an upstack lib

eerhardt · 2020-08-20T17:14:56Z

I don't think the wasm likes the ref address taking or gotos in the main method; adding a branch to move to a simple for loop method then doesn't get eliminated for the non-wasm platforms so will regress them.

A simple, top-level condition at the root of this method wouldn't get tier-JIT'd away?

public static int IndexOfQuoteOrAnyControlOrBackSlash(this ReadOnlySpan<byte> span)
{
    if (Sse2.IsSupported || AdvSimd.Arm64.IsSupported || Vector.IsHardwareAccelerated)
    {
        return IndexOfQuoteOrAnyControlOrBackSlashIntrinsic(span);
    }
    else
    {
        return IndexOfQuoteOrAnyControlOrBackSlashForLoop(span);
    }
}

benaadams · 2020-08-20T17:17:01Z

A simple, top-level condition at the root of this method wouldn't get tier-JIT'd away?

Arm would then go down the for loop path; rather than the unrolled path?

eerhardt · 2020-08-20T17:18:19Z

Arm would then go down the for loop path; rather than the unrolled path?

Arm32? Yes. Is that a concern?

benaadams · 2020-08-20T17:25:26Z

Arm would then go down the for loop path; rather than the unrolled path?

Arm32? Yes. Is that a concern?

It was highlighted as an issue #39733 (comment); I assume not regressing platforms when its not too hard to avoid is a desirable feature? /cc @jkotas

jkotas · 2020-08-20T17:36:53Z

We do support ARM32 and number of our customers are deploying on ARM32.

E.g. #41089 is about poor performance of System.Text.Json on Xamarin platforms that means ARM.

eerhardt · 2020-08-20T18:24:57Z

My uber concern here is unnecessarily building more upstack libraries for -Browser. In this case it is just for a single method. cc @ericstj

Maybe this is another case for RuntimeFeature.IsInterpreting (see #38439 (comment)). If running on an interpreter, it is slower to use an unrolled loop than a simple for, but running on JIT'd code it is faster to use an unrolled loop, then it would seem we should have some way to knowing which environment we are running on.

For 5.0, what if we used RuntimeFeature.IsDynamicCodeCompiled to check if we were interpreting (that will be the only case where IsDynamicCodeCompiled == false)? Then in 6.0 we make RuntimeFeature.IsInterpreting a real thing, and switch System.Text.Json over to use that instead?

jkotas · 2020-08-20T18:36:37Z

For 5.0

I do not see this change meeting the bar for backporting to release/5.0.

ericstj · 2020-08-20T18:37:03Z

We shouldn't be forking this library. Make this a runtime check please. I'm not an expert on the intricacies of getting this right so that the final JIT'ed code is still ideal perf, but other-places that needed some special behavior for interpreter were also using linker substitutions too, right?

I'd also like to block on @steveharter's feedback. He's out this week but should be able to take a look next week.

jkotas · 2020-08-20T18:40:13Z

running on JIT'd code it is faster to use an unrolled loop,

This is not necessarily true for non-optimized code. Maybe what we need is bool IsCurrentMethodOptimized { get; } that will be true/false depending on whether the current method body has codegen optimizations applied to it.

benaadams · 2020-08-20T18:49:44Z

I do not see this change meeting the bar for backporting to release/5.0.

For 6.0 will it run as precompiled wasm? Thus have different considerations?

jkotas · 2020-08-20T18:56:36Z

For 6.0 will it run as precompiled wasm?

That is the plan so far.

jeffhandley · 2020-08-24T20:37:41Z

I do not see this change meeting the bar for backporting to release/5.0.

I agree. I'm going to go ahead and move this to 6.0.0.

ericstj · 2021-03-08T17:33:42Z

@layomia / @eiriktsarpalis can you take a look here?

steveharter · 2021-03-12T17:13:55Z

| Slower | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson.Deseri | 1.07

@benaadams thoughts on this regression? Fluke or is something else like small payloads?

In general, the ~11% mean improvement is great!
better: 30, geomean: 1.113

src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs

benaadams · 2021-03-12T20:22:52Z

Rebased and applied feedback

src/libraries/System.Text.Json/src/System.Text.Json.csproj

eerhardt · 2021-03-12T21:55:43Z

src/libraries/System.Text.Json/src/System.Text.Json.csproj

+  <ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
+    <Compile Include="System\Text\Json\Reader\JsonReaderHelper.Intrinsics.cs" />
+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetFramework)' != '$(NetCoreAppCurrent)'">


Note that many of our libraries that are inbox and OOB do this:

<TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;netcoreapp3.0;net461</TargetFrameworks> <ExcludeCurrentNetCoreAppFromPackage>true</ExcludeCurrentNetCoreAppFromPackage>

This means that NetCoreAppCurrent is the build that goes in the runtimepack/shared framework. But in the NuGet package, only netstandard2.0;netcoreapp3.0;net461 builds get packaged in there. So that means if someone references the OOB System.Text.Json v7 NuGet package, even on net6.0, they will use the netcoreapp3.0 version from the package, because that's the highest version available.

So I really think these conditions should be '$(TargetFramework)' == '$(NetCoreAppCurrent)' or '$(TargetFramework)' == 'netcoreapp3.0' - Include .Intrinsics. Else - include .NoIntrinsics.

The intrinsics will break on netcoreapp3.0 though because they include the arm variants? Should I include net5.0 and treat that as the netcoreapp3.0 in your example?

This probably needs the experts.

@ericstj @steveharter @layomia - how do you want to handle Arm intrinsics in STJ? Do you want to ship a net5.0 TFM in the OOB package?

Could type forward, but not sure how that works either 😅

Type forwards wouldn't work here. I assume to support Arm we will need a new TFM in the package. But I think that's up to the System.Text.Json owners to decide. I'm just a fly-by reviewer 😄

The OOB packages make your app bigger and slower by design. For example, the OOB package does not come with R2R compiled code, so your app will startup slower when you reference the OOB package.

Not necessarily, I might build self-contained and run cross-gen myself. Or I might be running on an aot runtime.

If you want to depend on latest version, you better update the runtime too.

That's not always an option. Consider what happens in .NET 7.0. We're going to add API to this library and I bet the Azure SDK (or some other libs) will want to use it. At that point any 6.0 app that uses Azure SDK will have no choice but to use the package version.

I'd prefer we keep a simple rule here. If the configuration differs in conditions/ifdefs from the packaged configurations then it should also be included in the package.

At that point any 6.0 app that uses Azure SDK will have no choice but to use the package version.

Yes, it will continue to work - with small perf hit - even if we keep the status quo.

It is ok with me if we want build extra targets to ensure that newer OOB packages on downlevel platforms always get all performance tweaks possible.

There are other assemblies with this problem. For example,
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj uses ARM intrisics inbox, but there is no OOB build with ARM intrinsics. I assume that we would want to fix all these.

Consider what happens in .NET 7.0. We're going to add API to this library and I bet the Azure SDK (or some other libs) will want to use it. At that point any 6.0 app that uses Azure SDK will have no choice but to use the package version.

Sounds like an issue to resolve before next year's 7.0 release? 😉

Nothing gets resolved by that point. New API in the package means people will use that new API, so consuming the package that's newer than the framework is super common. Folks shouldn't have to pay a penalty when they do that. My position on this is don't use ExcludeCurrentNetCoreAppFromPackage if there is anything different about the NetCoreAppCurrent build. Don't split hairs on something saying it's OK because it's only a small perf fix. You might be able to convince yourself of that, but what about the next person that doesn't notice it's excluded? What about the customer that cares about that perf-fix.
The package growth problem is less of an issue now that we're starting to drop out of support frameworks from packages thanks to precedent that @ViktorHofer is setting.
Let's keep our rules simple: if you if'def/conditionally compile, then don't exclude from package.

Regarding the initial feedback, please don't set ExcludeCurrentNetCoreAppFromPackage anymore as we don't want to exclude the latest .NETCoreApp asset in packages anymore. See #53439 for more details.

As @ericstj mentioned, in the past we excluded specific tfms - even though they were shipping in some form - from our packages but based on the new package support policy we don't carry along unsupported .NETCoreApp assets along anymore which bounds the number of .NETCoreApp assets in a package.

src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Intrinsics.cs

benaadams · 2021-04-20T01:05:54Z

Is there more to do here?

tannergooding · 2021-04-20T16:48:36Z

src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.NoIntrinsics.cs

@@ -0,0 +1,193 @@
+// Licensed to the .NET Foundation under one or more agreements.


Why two copies of the file, rather than an internal shim for intrinsics that always returns false/throws?

Looks like I asked this a while back here: #41097 (comment) and then @steveharter asked for a file split here: #41097 (comment)

I am really not a fan of duplicating the Vector<T> and fallback logic across two files just so the intrinsic logic can be cleanly separated. The JIT (even full framework) should correctly handle dead code elimination for properties that simply return false and it will not be pleasant remembering to update both copies of the files for half the code.

Don't see any *.PlatformNotSupported.cs file references in System.Text.Encodings.Web.csproj?

Looks like @GrabYourPitchforks removed them in #49373

GrabYourPitchforks · 2021-04-21T00:48:11Z

@benaadams Can we get some extra unit test coverage on these APIs to help exercise edge cases in the SIMD / vectorized logic and prevent regressions? In particular, I'd like to see a unit test specifically exercise the IndexOfQuoteOrAnyControlOrBackSlash code path.

We have some types like BoundedMemory that can help with unit testing this. See Latin1UtilityTests.cs for some examples. The tests in that file create spans with interesting data at a bunch of different offsets to try to exercise edge cases in the SIMD logic, especially with regard to any pre-loop alignment or post-loop draining. The end of that file also shows using a delegate to invoke a byte*-consuming method, though in your case you'd probably want a ref byte-consuming method.

steveharter · 2021-06-01T16:27:52Z

@benaadams based on your timings this is an important PR. Do you plan on responding to @GrabYourPitchforks around potential regressions; if not we can have someone else investigate. Thanks

steveharter · 2021-07-08T17:48:22Z

The perf numbers look great, but at this point in the release it is risky to add this IMO. We should consider this for 7.0 early.

benaadams requested review from jozkee, layomia and steveharter as code owners August 20, 2020 15:48

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 20, 2020

benaadams mentioned this pull request Aug 20, 2020

Intrinsicify JsonReaderHelper.IndexOfOrLessThan #40877

Closed

eerhardt reviewed Aug 20, 2020

View reviewed changes

eng/testing/linker/trimmingTests.targets Outdated Show resolved Hide resolved

eerhardt reviewed Aug 20, 2020

View reviewed changes

eng/testing/linker/SupportFiles/Directory.Build.props Outdated Show resolved Hide resolved

layomia added area-System.Text.Json tenet-performance Performance related issue and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 20, 2020

layomia added this to the 5.0.0 milestone Aug 20, 2020

benaadams mentioned this pull request Aug 24, 2020

Optimize library code using arm64 intrinsics #33308

Closed

jeffhandley removed this from the 5.0.0 milestone Aug 24, 2020

ericstj assigned eiriktsarpalis and layomia and unassigned steveharter Mar 8, 2021

eiriktsarpalis assigned steveharter Mar 10, 2021

steveharter reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs Outdated Show resolved Hide resolved

src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs Show resolved Hide resolved

benaadams force-pushed the Json branch 2 times, most recently from 955f26f to 2241f2c Compare March 12, 2021 20:20

eerhardt reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System.Text.Json.csproj Outdated Show resolved Hide resolved

benaadams commented Mar 12, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System.Text.Json.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Mar 12, 2021

View reviewed changes

runfoapp bot mentioned this pull request Mar 15, 2021

SafeHandle use-after-dispose in FileSystemWatcher on OSX #30056

Closed

jkotas reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Intrinsics.cs Outdated Show resolved Hide resolved

Intrinsicify (x86, x64, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan

0ff68e7

benaadams force-pushed the Json branch from ab78254 to 0ff68e7 Compare March 19, 2021 00:39

benaadams mentioned this pull request Mar 19, 2021

Delete redundant vectorization code paths #49846

Closed

benaadams added 2 commits March 23, 2021 10:08

Merge branch 'main' into Json

686fa8e

Merge branch 'main' into Json

abf488f

tannergooding reviewed Apr 20, 2021

View reviewed changes

This was referenced Apr 21, 2021

ILLink is leaving in callsite to constant method dotnet/linker#1113

Closed

[wasm] SIMD support tracking issue #51709

Closed

karelz mentioned this pull request May 11, 2021

SyncHttpHandlerTest_AutoRedirect.AllowAutoRedirect_True_ValidateNewMethodUsedOnRedirection failed with "Socket is already connected" on macOS #50271

Closed

steveharter closed this Jul 8, 2021

ghost locked as resolved and limited conversation to collaborators Aug 7, 2021

		@@ -0,0 +1,193 @@
		// Licensed to the .NET Foundation under one or more agreements.

Intrinsicify (Sse, Axv2, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan #41097

Intrinsicify (Sse, Axv2, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan #41097

Conversation

benaadams commented Aug 20, 2020 • edited Loading

benaadams commented Aug 20, 2020

benaadams commented Aug 20, 2020

eerhardt commented Aug 20, 2020

benaadams commented Aug 20, 2020

benaadams commented Aug 20, 2020 • edited Loading

eerhardt commented Aug 20, 2020

benaadams commented Aug 20, 2020

eerhardt commented Aug 20, 2020

benaadams commented Aug 20, 2020

jkotas commented Aug 20, 2020

eerhardt commented Aug 20, 2020

jkotas commented Aug 20, 2020

ericstj commented Aug 20, 2020

jkotas commented Aug 20, 2020

benaadams commented Aug 20, 2020

jkotas commented Aug 20, 2020

jeffhandley commented Aug 24, 2020

ericstj commented Mar 8, 2021

steveharter commented Mar 12, 2021

benaadams commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks commented Apr 21, 2021

steveharter commented Jun 1, 2021

steveharter commented Jul 8, 2021

benaadams commented Aug 20, 2020 •

edited

Loading

benaadams commented Aug 20, 2020 •

edited

Loading

benaadams Apr 20, 2021 •

edited

Loading